311 fix run deletion#431
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
| try: | ||
| run = Run(run_name) | ||
| run.delete_run() | ||
| except Exception: | ||
| Run._instances.pop(run_name, None) | ||
| delete_run_folder(run_name) |
There was a problem hiding this comment.
I had a look at the delete_run() method of runs. It looks like this:
def delete_run(self) -> None:
delete_run_folder(self.run_name)
self._instances.pop(
self.run_name, None
) # remove instance from the class dictionary
So, the except is basically doing exactly the same as the try, apart from the different order? So, could you potentially just swap the order in delete_run or do something like:
def delete_run(self) -> None:
try:
delete_run_folder(self.run_name)
finally:
self._instances.pop(self.run_name, None)
There was a problem hiding this comment.
The difference between what happens in try is the instantiation of the run which we then delete. This is our "normal" case. However, if the run is broken (because for example it contains a step from another branch that this branch does not know) the instatiation will fail and that is when we land in exception.
The exception essentially does the same thing only it does not need to create the run. I guess what we could do is to not use delete_run at all and simply do it by ourselves without the need to create it, but the Run._instances.pop(run_name, None) feels more like something we only want to do in an exception and not as the normal case. But that is simply my feeling idk what do you say?
There was a problem hiding this comment.
I guess what I was trying to say is that simply changing delete_run will not do the trick because this is not where the problem lies.
There was a problem hiding this comment.
Thank you for the explanation. It makes sense to me now, so I would keep your implementation as is.
2 participants
Description
fixes #311
The bug is well-documented in the original issue, so I will focus on the fixes below.
Changes
As @tE3m already correctly identified, deletion was failing silently because the server tried to instantiate the run first, which breaks if the run is corrupted.
Now, if instantiation fails, we catch the error and force-delete the run directly, bypassing the need to instantiate the run in order to call delete_run.
I also changed the error message sent to the frontend slightly to better reflect what is actually going wrong.
Lastly, I also changed how the frontend handles deletions. Instead of expecting the deletion to work and already update the run table accordingly, we wait for the run to be successfully deleted. Only after that we actually update the table.
If the deletion fails, we now display an error for the user and don't fail silently.
Testing
Create a run while on the crosslinking branch and create some steps that are not on dev (e.g. the multimer importing step). Next, switch the branch to
311-fix-run-deletion. Here, try to delete the run. Reload and it should still be deleted. Create a new run with the same name, it should be a new run and not the old one again.To be honest, I am not sure how to test the error messaging because everything works :P
But of course feel free to stress test and break things.
PR checklist
Development
Mergeability
blackpnpm formatand checked withpnpm lintCode review